@tommymonk there are still some tests that need to be reverted. On the other hand, this PR is conflicted. Could you resolve the conflicts? So the CI works on this PR?

BTW, thanks for the calories burned on this PR!

I'm almost finished on the overloads the the Evaluate* methods but I've got test failures because the JToken now returned can 'empty' rather than null as confirmed by it's HasValues property. Would you rather I check for the HasValues property and return null, or task the calling code (tests and users) with checking this property ?

@tommymonk now the Evaluate* with not generic returns a Task<object>. I would by changing that to Task. So if you use EvaluateFunctionAsync("") we assume that you don't need the value. If you do need it, you should do EvaluateFunctionAsync<Foo>("").

We could also play smart and check if the user wants the raw JSON token EvaluateFunctionAsync<JToken>("") so if T is JToken we return the JSON object.

Unfortuantely I can't have overloads of these methods that return nothing (Task) as well as Task<JToken> because of course C# doesn't allow this with the same method signature.
I have chosen to go with Task<JToken> because then at least the caller can choose to not even acknowledge the return value

@kblok rebased atop your commits today

Is there a reason that you don't just use BaristaLabs to generate all your poco objects for each of the devtools APIs, seems like this would take care of Response types you're wanting ?

https://github.com/BaristaLabs/chrome-dev-tools-generator

@tommymonk, basically we were growing little by little not even knowing that this project would get any interest. We started with dynamics, and then we began moving to classes, adding small classes as we needed.
Now I have smart people like you suggesting apps I never heard before 😂
I will take a look at that app.

Removed line-ending changes to the files mentioned, merged in your master changes.

Just wanted to confirm you aren't waiting on me for any further rework ?

@tommymonk cool I didn't know it was ready again.

Besides my Evaluate* concerns, I think this is almost ready to go.
@Meir017 would you mind taking a look at it?

@tommymonk would you mind writing a Breaking Change report so we tell the users what it's changing for them and what they would need to change to get their app working?

@Meir017 let's try to merge this before any other PR, so we don't conflict our friend @tommymonk

I'm bound to have forgotten something, but will update this comment with your feedback.

Should we mention the change of Evaluate* return type changing from dynamic to JToken because the current type being boxed as dynamic is JToken, though ?
Same goes for Response.JsonAsync and CDPSession.SendAsync I guess ?

Use of evaluative methods that previously resolved to a JavaScript number will now return a more appropriate .NET type as discussed in #650

 @tommymonk think about the code that a user might need to change, so we add that to our release notes.

@tommymonk 💥

Thanks

I believe that I've found a bug introduced by my PR that isn't covered by test.

await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultRevision);

var browser = await new Launcher().LaunchAsync(new LaunchOptions
{
    Headless = true,
});

var page = await browser.NewPageAsync();

var result = await page.EvaluateExpressionAsync("var str = 'asdf'; str;"); // result is null

browser.Dispose();

The return type of EvaluateExpressionAsync isn't specified, the object is string and it return null

Its cause because of this line of code in ExecutionContext;
return result is JToken token && !token.HasValues ? default : result;

Instead of just checking HasValues it needs to use a smarter test like the top answer here

public static bool IsNullOrEmpty(this JToken token)
{
    return (token == null) ||
           (token.Type == JTokenType.Array && !token.HasValues) ||
           (token.Type == JTokenType.Object && !token.HasValues) ||
           (token.Type == JTokenType.String && token.ToString() == String.Empty) ||
           (token.Type == JTokenType.Null);
}

@tommymonk go for it an add a test :)

My branch is gone now so I'll create a new one and new PR ?

@tommymonk yes

I'll get on it, can you suggest where is appropriate fore new tests to go ?

@tommymonk I would go with PageTests.EvaluateTests.ShouldWorkWithoutGenerics()